-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Example color theme improvements #5087
Example color theme improvements #5087
Conversation
…ts / restrictions
There are some unexpected Percy changes, it looks like something I changed in the SCSS has had some unintended consequences. Investigating now |
Removed example sass from vanilla bundle and now there are no percy diffs. |
2c56fbe
to
9ee4632
Compare
9ee4632
to
9f766dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some leftovers from previous implementation that may be not needed anymore.
@lyubomir-popov Your feedback is now in the demo; see button for example |
67bbe5a
to
4dce656
Compare
4dce656
to
16e4ec3
Compare
Not directly related to the color theming toggle functionality, but I notice some components still don't fully support all the themes - for example: Does this affect our desire to add this theme toggle right now? Seems like maybe it should be held off until all the components fully support each theme? |
@pastelcyborg We're not adding the theme toggle - it's currently in production. The problem with it is that it doesn't reflect the active state (the selected color theme is not shown on the theme selector). See comment by Bartek for context |
The theme switcher is already there, this PR only makes it better. |
There are some z-index issues (with notifications for example). https://vanilla-framework-5087.demos.haus/docs/examples/layouts/docs Even more visible here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Done
More limited version of #5086 that achieves #5086 goals without modifying the back-end or modifying example templates.
Fixes #5069, WD-11110
QA
div.u-example-controls
) is positioned to the bottom right of the page, not its contents.theme
has been set to your selected theme.theme
query parameter is present in your URL.Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots